Include runtime stderr in error messages#149
Open
arpitjain099 wants to merge 1 commit into
Open
Conversation
5cce03a stopped folding stderr into stdout to keep Bun's .env loading noise out of the parsed output (rails#130). A side effect was that when the runtime exits non-zero, the diagnostic it writes to stderr (parse errors, stack traces) is dropped, so the raised ExecJS::RuntimeError carries no useful detail (rails#142). Capture stderr separately by redirecting the child's stderr to a temp file (a file, not a pipe, so there is no risk of a deadlock between draining stdout and stderr), and include it in the error only on the failure path. The success path still reads stdout exactly as before, so rails#130 stays fixed. exec_runtime_error takes an optional stderr argument and is backwards compatible, so the Windows branch (which uses its own redirect) is unaffected. The two IO.popen branches that 5cce03a changed (default and JRuby) are the ones updated here. Adds a regression test that a non-zero runtime exit surfaces its stderr. Fixes rails#142 Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #142. When the external runtime exits non-zero, the diagnostic it writes to stderr (parse errors, stack traces) was being dropped, so
ExecJS::RuntimeErrorcarried no useful detail.5cce03a removed the stderr-into-stdout merge to keep Bun's
.envloading noise out of the parsed output (#130). This keeps that property: it captures stderr separately by redirecting the child's stderr to a temp file (a file, not a pipe, so there is no deadlock risk between draining stdout and stderr) and includes it in the error only on the failure path. The success path still reads stdout exactly as before, so #130 stays fixed.exec_runtime_errornow takes an optionalstderrargument and is backwards compatible, so the Windows branch (which uses its own2>&1 > fileredirect) is unaffected. The twoIO.popenbranches that 5cce03a changed (default and JRuby) are the ones updated here.Test
Adds
test_runtime_error_includes_stderr, asserting a non-zero runtime exit surfaces its stderr. Full suite is green locally on MRI 2.6 + Node; the JRuby branch mirrors the same change and relies on CI.ExecJS::RuntimeErrormessage was empty/uninformative on a runtime parse error.SyntaxErrortrace).